Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to run on an ec2 instance #272

Merged
merged 7 commits into from
Mar 7, 2024
Merged

Conversation

lajohn4747
Copy link
Contributor

resolves #265

Add the field run_on_ec2 which spins up an EC2 instance with the environment credentials, runs the latest version of sdgym with the same parameters and saves the output to the s3 bucket. Terminates the Ec2 instance once finished with the process to avoid keeping paid resources alive.

Only issue is that custom synthesizers may not work with this command in this PR (as that requires a standardized way to bring in custom imported classes to the EC2 instance as well)

@lajohn4747 lajohn4747 requested a review from a team as a code owner March 6, 2024 17:22
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just minor comments/questions

sdgym/benchmark.py Outdated Show resolved Hide resolved
sdgym/benchmark.py Show resolved Hide resolved
sdgym/benchmark.py Outdated Show resolved Hide resolved
sdgym/benchmark.py Outdated Show resolved Hide resolved
sdgym/benchmark.py Outdated Show resolved Hide resolved
@lajohn4747 lajohn4747 marked this pull request as draft March 6, 2024 21:47
sdgym/benchmark.py Outdated Show resolved Hide resolved
@lajohn4747 lajohn4747 marked this pull request as ready for review March 6, 2024 23:11
sdgym/benchmark.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 50.04%. Comparing base (1d55575) to head (41bf69b).
Report is 1 commits behind head on main.

Files Patch % Lines
sdgym/benchmark.py 73.52% 9 Missing ⚠️
sdgym/cli/__main__.py 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
+ Coverage   49.39%   50.04%   +0.64%     
==========================================
  Files          19       19              
  Lines        1166     1201      +35     
==========================================
+ Hits          576      601      +25     
- Misses        590      600      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sdgym/benchmark.py Outdated Show resolved Hide resolved
Comment on lines +530 to +531
ImageId='ami-07d9b9ddc6cd8dd30',
InstanceType='t2.medium',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be configurable arguments on the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one I was wondering about. This requires, the user to put in a lot more information about the configuration. The block mapping in line 548, those fields depend on the image. Instance type depends on the image. I think it's fine to hard code because these configurations exist and would require different scripts for each type of VM and errors on the ec2 instance (not enough memory, incorrect os script, etc.) are pretty much on the user to debug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave out configuration for now and add it later if we need to

sdgym/benchmark.py Show resolved Hide resolved
@lajohn4747 lajohn4747 requested a review from gsheni March 7, 2024 17:45
Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@lajohn4747 lajohn4747 merged commit 3e126d7 into main Mar 7, 2024
45 checks passed
@lajohn4747 lajohn4747 deleted the issue_265_run_on_ec2 branch March 7, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add run_on_ec2 flag to benchmark_single_table
4 participants